Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Fix subject hierarchy table showing nodes removed during batch processing #6394

Merged
merged 1 commit into from
Sep 20, 2022
Merged

BUG: Fix subject hierarchy table showing nodes removed during batch processing #6394

merged 1 commit into from
Sep 20, 2022

Conversation

jamesobutler
Copy link
Contributor

@jamesobutler jamesobutler commented May 26, 2022 •
edited

Originally reported at https://discourse.slicer.org/t/optimizing-performance-of-events-when-clearing-nodes/23588/9.

Code to replicate original issue

slicer.util.selectModule("Data")
first = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "First")
second = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Second")
third = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Third")
slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)

for node in [third, second]:
  slicer.mrmlScene.RemoveNode(node)

slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
fourth = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Fourth")

After running the above code:

Slicer 5.1.0-2022-05-24 This PR
image image

I have removed updateFromSubjectHierarchy because of it's single usage in Slicer core being removed. This could be reverted if it is desired even though it is code not being used by core. onMRMLSceneEndBatchProcess was switched to using the full rebuildFromSubjectHierarchy as when removing nodes during batch processing it was not removing them from the widget when batch processing state was ended.

When using Slicer 5.1.0-2022-05-24 and the subject hierarchy widget not updating it would produce many errors of the following when the batch processing state was ended:

[DEBUG][Qt] 26.05.2022 13:03:25 [] (unknown:0) - Python console user input: slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 26.05.2022 13:03:25 [vtkMRMLSubjectHierarchyNode (000002CDB734A7B0)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9

@pieper
Copy link
Member

pieper commented May 26, 2022

LGTM, but I think @cpinter should have a look.

@jamesobutler jamesobutler requested a review from cpinter May 27, 2022 18:24
@cpinter
Copy link
Member

cpinter commented May 31, 2022

Thanks for working on this @jamesobutler !

I'm not opposed to removing this function if it's really not necessary, but having a way to soft update SH still seems like a good idea. Out of curiosity: have you tried improving the update function to remove these items?

Have you noticed a decrease in the other SH related error messages after your change? Unfortunately there are still some, like about item ID mismatch, cache inconsistency etc.

@jamesobutler
Copy link
Contributor Author

jamesobutler commented Jun 9, 2022 •
edited

Out of curiosity: have you tried improving the update function to remove these items?

@cpinter I have attempted this, but the subject hierarchy code has been pretty confusing for me to understand between all the item terminology. Do you have any thoughts how to access these items where are no longer in the subject hierarchy node, but are persisting in the model of the various table views observing the subject hierarchy?

For example the following where I remove all items in the subject hierarchy after exiting batch process state, still results in items in the table view. This when still using the updateFromSubjectHierarchy.

slicer.util.selectModule("Data")
first = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "First")
second = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Second")
third = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Third")
slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)

for node in [third, second]:
  slicer.mrmlScene.RemoveNode(node)

slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
fourth = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Fourth")
shNode = slicer.mrmlScene.GetSubjectHierarchyNode()
shNode.RemoveAllItems()

image

@jamesobutler
Copy link
Contributor Author

@cpinter I could keep updateFromSubjectHierarchy, but remove the early return in the BatchProcessState when items are being removed from the subject hierarchy. Would mean processing the removal and reparenting as it goes instead of doing it once out of the BatchProcessState.

Slicer/Modules/Loadable/SubjectHierarchy/Widgets/qMRMLSubjectHierarchyModel.cxx

Lines 1521 to 1527 in d24cd5b

void qMRMLSubjectHierarchyModel::onSubjectHierarchyItemAboutToBeRemoved(vtkIdType itemID)
{
Q_D(qMRMLSubjectHierarchyModel);
if (d->MRMLScene->IsClosing() || d->MRMLScene->IsBatchProcessing())
{
return;
}

Slicer/Modules/Loadable/SubjectHierarchy/Widgets/qMRMLSubjectHierarchyModel.cxx

Lines 1553 to 1560 in d24cd5b

void qMRMLSubjectHierarchyModel::onSubjectHierarchyItemRemoved(vtkIdType removedItemID)
{
Q_D(qMRMLSubjectHierarchyModel);
Q_UNUSED(removedItemID);
if (d->MRMLScene->IsClosing() || d->MRMLScene->IsBatchProcessing())
{
return;
}

Would produce the expected result as well.
image

@cpinter
Copy link
Member

cpinter commented Jun 10, 2022

pretty confusing for me to understand between all the item terminology

Are you familiar with Qt's model/view architecture? If not, then either start from there (and then find the Qt model items corresponding to non-existent SH items and remove them from the Qt model), or consider this issue done.

Would mean processing the removal and reparenting as it goes instead of doing it once out of the BatchProcessState.

Yes, but the whole point of batch processing is to postpone these updates until the end so that the processing itself is fast when you know that many objects are to be processed.

Btw I don't see images uploaded in GitHub comments for around two days now. Hopefully it will be fixed, otherwise it will be hard to follow discussions without seeing the illustrations...

image

@jamesobutler
Copy link
Contributor Author

I'm not sure what is going on your end with the pictures. The pictures are rendering successfully for me on GitHub in my browser and on my mobile device.

Currently I have limited time with this improvement so if removing of updateFromSubjectHierarchy and using rebuildFromSubjectHierarchy is not a valid improvement then we can close this PR and let this problem of batch processing to persist until it can be fixed in another way.

@cpinter
Copy link
Member

cpinter commented Jul 7, 2022

Sorry this has fallen through the cracks. I'm OK with integrating this. My only problem is that I'm not completely sure what other consequences it will have that we rebuild the hierarchy from scratch - haven't had the time to do thorough testing. I guess we can integrate this and see if it causes problems. If it does it will occur quickly because we all use SH to overview and handle the loaded data.

@cpinter
Copy link
Member

cpinter commented Sep 7, 2022

@jamesobutler @lassoan If you don't see any issues please go ahead with integration whenever appropriate.

@cpinter cpinter merged commit 033eddb into Slicer:main Sep 20, 2022
@jamesobutler jamesobutler deleted the batch-processing-fixes branch September 20, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpinter cpinter cpinter approved these changes

@pieper pieper Awaiting requested review from pieper

@lassoan lassoan Awaiting requested review from lassoan

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jamesobutler @pieper @cpinter